Skip to content

More changes needed for NetworkX plugin #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 1, 2022

Conversation

jim22k
Copy link
Member

@jim22k jim22k commented Oct 28, 2022

  • k-truss returns a normal Graph object
  • Rename convert to convert_from_nx
  • Add convert_to_nx method
  • Make algorithms accept Graph, Matrix, or Matrix-variant (expression, transpose, etc)

- k-truss returns a normal Graph object
- Rename convert to convert_from_nx
- Add convert_to_nx method
- Make algorithms accept Graph, Matrix, or Matrix-variant (expression, transpose, etc)
@jim22k jim22k requested a review from eriknw October 28, 2022 21:54
@eriknw
Copy link
Member

eriknw commented Oct 28, 2022

We should probably add a utility function in python-graphblas to help convert expressions to objects. In python-graphblas, we frequently use A._expect_type_message(x, Matrix, ...) (and sometimes output_type(x)) to do this internally, which also raises informative error message if applicable. I think a simpler function would be nice. Should it respect "autocompute" config option?

  • A = gb.ensure(A, gb.Matrix)
    • This is explicit, but what if we want Matrix or TransposedMatrix? Need to pass in both?
  • A = gb.core.ensure_matrix(A), ensure_vector, ensure_scalar
    • Returns a Matrix or TransposedMatrix, computing if necessary
    • Can have allow_transpose= keyword argument (default True)

This can also have compute=None that defers to the "autocompute" config if unspecified.

I'll give this more thought and will try to give a concrete proposal of my preference.

@eriknw
Copy link
Member

eriknw commented Oct 29, 2022

Alright, here's my proposal, ensure_type(x, types): python-graphblas/python-graphblas#312

@jim22k
Copy link
Member Author

jim22k commented Oct 29, 2022

I like the ensure_type proposal. Let's merge that in python-graphblas and then use it here.

@eriknw
Copy link
Member

eriknw commented Oct 29, 2022

What can we do about the return type of generalized_degree? Our native return type is Matrix, but networkx is a dict of dicts, {row: {col: val}}. Perhaps we should pass the current function dispatch name to convert_to_nx.

@jim22k
Copy link
Member Author

jim22k commented Oct 29, 2022

For generalized_degree, we probably need to take the approach of returning a Graph from graphblas-algorithms, and then return a dict of dicts from the interface.

I know the goal is to avoid translation costs when using dispatching in "real" code, so returning a Graph from k_truss is ideal. However, if someone has code today which calls generalized_degree, they don't get back an nx.Graph, so the idea of passing the returned value from generalized_degree to k_truss (for example) isn't an option. They would expect exactly a dict of dicts.

I think we need to follow suit and return a dict of dicts so their code "just works".

But we should also raise an issue with NetworkX and ask why that is the return type. If there is a good reason, okay keep it as is. But if not, having a Graph object feels so much more practical.

@eriknw
Copy link
Member

eriknw commented Oct 29, 2022

Sorry, I misspoke. The NetworkX dict for generalized_degree is of the form {node: {degree: count}}, where node is the generic node label, degree is an integer that reflects the degree of a neighbor, and count is an integer of how many neighbors have that degree. So, this isn't quite a graph. It's more like a NodeMap where the values are dicts.

Our native return time is naturally a Matrix; see the magic here (from_values does heavy lifting):

Tri(Tri.S, binary.second) << plus_pair(Tri @ A.T)
rows, cols, vals = Tri.to_values()
# The column index indicates the number of triangles an edge participates in.
# The largest this can be is `A.ncols - 1`. Values is count of edges.
return Matrix.from_values(
rows,
vals,
np.ones(vals.size, dtype=int),
dup_op=binary.plus,
nrows=A.nrows,
ncols=A.ncols - 1,
name="generalized_degree",
)

So, I added a simple fix to matrix_to_dicts in #27, because our current conversion was wrong for non-default node ids. Perhaps we can also add matrix_to_nodemap conversion that can be treated as a dict of dicts and would work for generalized_degrees.

The more I've thought about it, the more I think we should add the function dispatch name to both convert_to_nx and convert_from_nx. This is pragmatic. In an ideal world where we have infinite time, we probably wouldn't need this. But, I think this will also be useful for the cugraph plugin.

@eriknw
Copy link
Member

eriknw commented Oct 30, 2022

Here's another argument for adding function dispatch name to test convert functions:

Some NetworkX functions/algorithms return numpy array or scipy.sparse array, but we'll want to return GraphBLAS vector or matrix. I think converting them during tests is the best option--we don't want to be forced to make our objects "look" like numpy or scipy.sparse, and it would be awkward (and difficult) to create a wrapper for them to look like numpy or scipy.sparse.

@eriknw
Copy link
Member

eriknw commented Nov 1, 2022

We should use convert_to_nx for tournament_matrix to convert to scipy.sparse.

@codecov-commenter
Copy link

Codecov Report

Base: 72.80% // Head: 73.63% // Increases project coverage by +0.83% 🎉

Coverage data is based on head (e48cec8) compared to base (4bb7cfb).
Patch coverage: 52.63% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   72.80%   73.63%   +0.83%     
==========================================
  Files          68       68              
  Lines        2335     2348      +13     
  Branches      430      428       -2     
==========================================
+ Hits         1700     1729      +29     
+ Misses        477      459      -18     
- Partials      158      160       +2     
Impacted Files Coverage Δ
graphblas_algorithms/classes/digraph.py 39.85% <0.00%> (-0.40%) ⬇️
graphblas_algorithms/classes/graph.py 58.85% <0.00%> (-0.57%) ⬇️
graphblas_algorithms/classes/_utils.py 76.25% <100.00%> (+0.34%) ⬆️
graphblas_algorithms/interface.py 97.29% <100.00%> (+0.23%) ⬆️
graphblas_algorithms/nxapi/core.py 100.00% <100.00%> (ø)
graphblas_algorithms/algorithms/core.py 100.00% <0.00%> (ø)
...aphblas_algorithms/nxapi/shortest_paths/generic.py 80.00% <0.00%> (+30.00%) ⬆️
...as_algorithms/algorithms/shortest_paths/generic.py 85.18% <0.00%> (+70.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +87 to +91
def convert_to_nx(obj, *, name=None):
from .classes import Graph

if isinstance(obj, Graph):
obj = obj.to_networkx()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check isinstance(obj, Matrix) and convert it to a scipy sparse array if scipy is installed?

We can also merge this PR and update things later.

@jim22k jim22k merged commit 3b113f5 into python-graphblas:main Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants